Codecov Report

❌ Patch coverage is 83.22981% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 45.06%. Comparing base (9c90a5d) to head (31b4bd7).
⚠️ Report is 18 commits behind head on nicka/initial-deposit-32.

@@                     Coverage Diff                      @@
##           nicka/initial-deposit-32    #2905      +/-   ##
============================================================
- Coverage                     50.83%   45.06%   -5.78%     
============================================================
  Files                           110      110              
  Lines                          4873     4920      +47     
  Branches                       1353     1362       +9     
============================================================
- Hits                           2477     2217     -260     
- Misses                         2392     2699     +307     
  Partials                          4        4              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

Claude's review

🔴 Blocker — 1. New implementation exceeds the EIP-170 size limit; migration 196 will fail to deploy on mainnet

CompoundingStakingSSVStrategy deployed (runtime) bytecode at HEAD = 25,077 bytes vs the 24,576-byte EIP-170 limit → over by 501 bytes, using
the repo's committed compiler settings (solc 0.8.28, optimizer.enabled = true, default runs, no overrides in hardhat.config.js). The
currently-deployed mainnet impl is 24,223 bytes.

Tests/compile pass locally only because allowUnlimitedContractSize: true is set on the hardhat network — but mainnet enforces EIP-170 at deploy
time, so deployWithConfirmation("CompoundingStakingSSVStrategy", …) inside migration 196 would revert on mainnet.

Worth chasing: the committed config produces 25,077 bytes locally, yet the deployed mainnet artifact is only 24,223 bytes (854 smaller) — implying the
production pipeline historically used different/more-aggressive optimizer settings than what's committed, or the contract has grown since.

Action: Before queuing 196, compile under the exact production profile and confirm runtime bytecode < 24,576 (CONTRACT_SIZE=true pnpm hardhat compile). If over, raise optimizer.runs / add a per-contract override / viaIR, or move logic into a library.

🟠 High — 2. Off-chain stakeValidator skips BLS signature verification for first deposits below the cap

The contract treats any deposit to a REGISTERED validator as the validator-creating deposit (amount in [1 ETH, cap]). The beacon chain only checks
the BLS signature on that creating deposit; top-ups are not signature-checked. But the task gates verification on exact equality:

const amountWei = parseUnits(amount.toString(), 18);
const initialDepositAmountWei = await strategy.initialDepositAmountWei();
if (amountWei.eq(initialDepositAmountWei)) { /* verify real BLS sig */ }
else { /* substitute dummy 0x…0001 signature */ }

A first deposit of any amount below the cap (now a supported path, exercised by the new "Should stake less than the initial deposit amount" test)
takes the else branch and submits a dummy signature for a validator-creating deposit → the validator is never created and the ETH is stuck/lost.

Concretely reachable via the P2P/uuid flow: utils/p2pValidatorCompound.js hardcodes INITIAL_DEPOSIT_SIZE = "32000000000" (32 ETH), and the
--uuid path overwrites amount with it (tasks/validatorCompound.js:165). Migration 196 sets the cap to 32.25 ETH. 32 ≠ 32.25 → the task strips
the real P2P signature and submits a dummy one for the activating deposit.

Action: Detect the first/creating deposit by on-chain validator state (REGISTERED), not amount equality, and verify the real BLS signature whenever
state is REGISTERED regardless of amount. Align INITIAL_DEPOSIT_SIZE with the on-chain cap (or drive it from initialDepositAmountWei). Add an
integration test of the uuid path at the raised cap.

🟡 Medium — 3. Reusable upgradeCompoundingStakingSSVStrategy helper is a bare upgrade → initialDepositAmountWei == 0 DoS

The shared helper (deployActions.js:209–243, used by deploy/hoodi/003,006,007,009,010) does only upgradeTo(...) with no setInitialDepositAmount. On
any already-initialized proxy upgraded through it, the new slot defaults to 0; since first deposits require both >= 1 ether and <= initialDepositAmountWei, those constraints become mutually unsatisfiable → every new-validator first deposit reverts "Invalid first deposit amount"
until a governor sets the cap. Mainnet is safe (196 sets it atomically), but this is a footgun for future/testnet upgrades.

Action: Have the helper set a sane default (e.g. 1 ETH) if initialDepositAmountWei == 0 post-upgrade, or assert it's non-zero.

🟡 Medium — 4. Test coverage gaps

• No stakeEth-level test of the production config. The renamed "…above the initial deposit amount" test never calls setInitialDepositAmount, so it
  only checks the cap against the default 1 ETH — same as the old == 1 ETH test. Add a test that raises the cap to 32.25 ETH and asserts a 32.25 ETH
  first deposit succeeds / >32.25 reverts.
• No fork test for migration 196 (the PR's own "Fork tests that test after the deployment file runs" box is unchecked). Should assert
  initialDepositAmountWei() == 32.25 ETH post-proposal and that adjacent storage survives the gap shrink.
• The off-chain sub-cap-signature path (Local Compound #2) has no test.

🟢 Low / Info / Nit

• Stale storage-layout snapshot: storageLayout/mainnet/CompoundingStakingSSVStrategy.json still shows depositedWethAccountedFor → __gap[41] with no
  initialDepositAmountWei, and is not in the diff. Self-heals on mainnet deploy, but until then it can't catch a slot shift in review. Regenerate and
  commit.
• PR description doesn't quantify the security trade-off: raising the cap moves worst-case per-incident front-run exposure 1 → 32.25 ETH (~32×). Worth
  one sentence + the mitigation (single-pending firstDeposit guard + resetFirstDeposit).
• 32× per-incident exposure is an intentional, time-limited tradeoff; aggregate exposure stays bounded to one validator, front-run funds are correctly
  non-recoverable and accounted (verifyValidator:651–680). Ensure monitoring + resetFirstDeposit/pause runbooks are ready.
• PlantUML diagram now says "1 ETH" — consistent with the default but doesn't depict the 32.25 ETH single-deposit window. Intentional per commit
  bb69b1e7a; consider a footnote.
• ValidatorState.ACTIVE comment says "at least 32.25 ETH" but verifyBalances uses strict > (:1122) — should read "more than 32.25 ETH".
  Pre-existing; only the number changed here.

Thanks for your review @sparrowDom

1. I've got the contracts back under size. I've had to use more custom errors. These are not used by external users so will be ok.
2. I've pushed a fix to the stakeValidator Hardhat task
3. This upgrade helper is not used by the deploy script but I've updated it anyway.
4. It's hard to fork test the consolidation to the new compounding staking contract when the contract or validators do not yet exist. We only have three more consolidations from the old NativeStakingStrategy. We could create unit tests with mocked beacon chain behaviour, but the beacon chain behaviour is key.

🟠 N-1 (residual H2) — stakeValidator --consol true submits a dummy BLS signature for a creating deposit

contracts/tasks/validatorCompound.js:174-216 (logic from dd4a4f98 / #2914, both post-review).

The task hardcodes strategy = CompoundingStakingSSVStrategyProxy and computes isCreatingDeposit = (strategy.validator(pubKeyHash).state == REGISTERED). But when --consol true, contract.stakeEth(...) routes through ConsolidationController.stakeEth (ConsolidationController.sol:469-479) to targetStrategy (the new vanilla CompoundingStakingStrategy), whose creating-deposit state is NON_REGISTERED (0), not REGISTERED (1). So for a real creating deposit on the target strategy the check is false → the task substitutes the dummy 0x…0001 signature → the validator is never created and the ETH is parked in a non-activating pending deposit (recoverable only via the withdrawal credentials).

--consol true is the only task path that can stake the new target strategy (the SSV proxy is hardcoded for the read), so this is the path that matters for creating target validators.

Fix: when consol, source the validator state (and the creating-deposit decision) from the target strategy — the controller already exposes it (ConsolidationController.sol:93 reads targetStrategy.validator(...)). Verify the real BLS signature whenever the target validator is in its creating state, regardless of amount. Add a test that drives the consol creating-deposit path.

🔵 N-2 (Low) — 197 removeStrategy relies on an unenforced, unsimulated lastVerifiedEthBalance ≈ 0

contracts/deploy/mainnet/197_remove_old_compounding_ssv_strategy.js (new file, #2914).

197's second action calls Vault.removeStrategy(oldCompoundingSSVStrategyProxy), which runs withdrawAll() then requires checkBalance(WETH) < maxDustBalance (1e13). checkBalance = lastVerifiedEthBalance + WETH.balanceOf(strategy). withdrawAll drains ETH/WETH but doesn't independently zero lastVerifiedEthBalance; if exited-validator ETH hasn't yet landed and been swept at execution time, the action reverts and blocks the whole proposal. 197 only guards on SSV validatorCount == 0. On fork, validators are forced EXITED via raw storage writes without setting lastVerifiedEthBalance, so the real precondition is never exercised.

Fix: before executing 197 on mainnet, assert off-chain checkBalance(WETH) < 1e13 and run verifyBalances/withdrawAll so lastVerifiedEthBalance is reduced first. Optionally add a fork test driving exit→verify→withdrawAll→removeStrategy.

⚪ N-3 (Info) — overloaded NotRegisteredOrVerified error gives a misleading revert on the register path

contracts/contracts/strategies/NativeStaking/CompoundingStakingSSVStrategy.sol:71-72 (re-added by #2915).

The re-added guard if (validator[pubKeyHash].state != NON_REGISTERED) revert NotRegisteredOrVerified(); reverts because the validator IS already registered, yet the error name asserts the opposite. The same error is correctly used in _admitStake. Off-chain consumers can't distinguish "already registered" from "not yet registered/verified." Guard logic is correct (tests confirm the revert).

Fix (optional): add a distinct error AlreadyRegistered(); for the register path.

4. Pre-existing issue surfaced now (not introduced since review)

🟡 M-1 — Legacy NativeStakingSSVStrategy.withdrawSsvClusterEth sweeps the entire ETH balance, ignoring consensusRewards

contracts/contracts/strategies/NativeStaking/NativeStakingSSVStrategy.sol:131-143. Present at the 2026-06-02 review (file is byte-identical REVIEW..HEAD; introduced 06-01 by cf8461db). Not new — a finding missed the first time.

ISSVNetwork(SSV_NETWORK).withdraw(operatorIds, amount, cluster);
uint256 ethBalance = address(this).balance;   // entire balance, not `amount`
if (ethBalance > 0) {
    IWETH9(WETH).deposit{ value: ethBalance }();
    IERC20(WETH).safeTransfer(vaultAddress, ethBalance);
    emit Withdrawal(WETH, address(0), ethBalance);
}

On the legacy strategy address(this).balance is not only cluster ETH: it can include accounted consensusRewards and un-accounted exit/withdrawal ETH. The sweep never decrements consensusRewards, so a later doAccounting() can see balance < consensusRewards → _failAccounting → pause (strategist-recoverable via manuallyFixAccounting). No fund loss (ETH reaches the legitimate vault), but it mistimes yield (bypasses the Dripper) and can overstate checkBalance. The sibling CompoundingStakingSSVStrategy.withdrawSsvClusterEth routes through _withdraw/_convertEthToWeth and stays consistent.

Reachability: onlyGovernor; 196 upgrades NativeStakingSSVStrategy2Proxy to the impl exposing it; no included script calls it on the legacy strategy — reachable only via a future governance proposal while the strategy holds reward/withdrawal ETH.

Fix: restrict the sweep to the amount withdrawn (snapshot before/after) and route through the strategy's normal accounting path, or require consensusRewards == 0 / run doAccounting first. Mirror the compounding sibling.

Thanks @sparrowDom

N-1 (residual H2) — stakeValidator --consol true submits a dummy BLS signature for a creating deposit
Has been fixed in PR #2918
I've also added a new ssv option that defaults to false to all the compounding staking strategy Hardhat tasks.

N-2 (Low) — 197 removeStrategy relies on an unenforced, unsimulated lastVerifiedEthBalance ≈ 0
This won't be a problem as all the compounding validators have been exited and swept.

N-3 (Info) — overloaded NotRegisteredOrVerified error gives a misleading revert on the register path
Has been fixed in PR #2919

M-1 — Legacy NativeStakingSSVStrategy.withdrawSsvClusterEth sweeps the entire ETH balance, ignoring consensusRewards
FIxed in PR #2920